overlord/snapstate: add migration function to fix invalid channel spec #7364
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, snapd performed no validation of the channel spec on refresh and install and just passed the string to the store for validation, however now we have to handle model constraints amongst other things and so we check the channel spec on refresh/update. This is problematic for clients that may have installed snaps using an unsupported channel spec with a leading "/" as in "/<risk>" which either implied "latest/<risk>" or as in "/<track>" which implied just "<track>". This patch fixes the problem "on the fly" when we encounter such a channel spec in case it exists in some deployments and corrects it by removing the leading "/" in order to keep the same expected behavior such that "/stable" gets changed to "stable" which has a defined semantic around it. Notably, we do this before calling resolveChannel, because we don't want to have that function grow too complicated so it can be more generally useful and instead just patch the snap state before that function gets called so it "just works" by the time we get there. The migration function also returns the new channel so that the in memory snapst can be updated to match what's now in the state so there are no inconsistencies later on, otherwise we will get change conflicts. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
This test ensures that old branch specs are automatically upgraded to the current supported spec. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
|
@chipaca is looking at a more holistic approach to this and also providing warnings |
|
we are working on a lower level patch and also all other related code has been reverted for now |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Note, I wasn't able to finish the following things that perhaps should be done before merging this, so feel free to take over this branch and clean up as necessary while I'm offline:
/edinburgh->edinburgh)This is the second part of the fix for https://bugs.launchpad.net/snapd/+bug/1841475
Previously, snapd performed no validation of the channel spec on refresh and install and just passed the string to the store for validation, however now we have to handle model constraints amongst other things and so we check the channel spec on refresh/update. This is problematic for clients that may have installed snaps using an unsupported channel spec with a leading "/" as in "/" which either implied "latest/" or as in "/" which implied just "".
This patch fixes the problem "on the fly" when we encounter such a channel spec in case it exists in some deployments and corrects it by removing the leading "/" in order to keep the same expected behavior such that "/stable" gets changed to "stable" which has a defined semantic around it.
Notably, we do this before calling resolveChannel, because we don't want to have that function grow too complicated so it can be more generally useful and instead just patch the snap state before that function gets called so it "just works" by the time we get there.
The migration function also returns the new channel so that the in memory snapst can be updated to match what's now in the state so there are no inconsistencies later on, otherwise we will get change conflicts.